New severity levels for form/record validations + enhanced FormField styling
#4194
Conversation
ghsolomon
changed the title
| /** Array of all validations for this record. */ | ||
| get allValidations(): Validation[] { | ||
| return flatMap(this.validations); | ||
| } |
There was a problem hiding this comment.
Would it be worth it to improve the names of both the new and pre-existing errors getters? We could add deprecations.
Could take allErrors -> errorsList and/or errors -> errorsMap / errorsByField. (I would vote for the latter as it's ...not a map)
My annoyance here is that we have this other strong convention in store where all means "unfiltered" - which seems very natural - and that's not what this is. Before we extend the existing pattern, would like to consider updating for clarity.
|
Thanks for this, Greg! Realize it's a lot to corral across forms + stores, different inputs, desktop + mobile. |
|
My assumption had been that we would not want to introduce breaking changes/deprecations for this. |
| export type RecordValidationResultsMap = Record<string, ValidationResult[]>; | ||
|
|
||
| /** Map of Field names to Field-level validation message lists. */ | ||
| export type RecordValidationMessagesMap = Record<string, string[]>; |
There was a problem hiding this comment.
In a separate pass - not this PR - I would like to look at consolidating down to a single /data/Types.ts file that could include all data-package types.
Would also like to add one or two little convenience alias types like such as FieldName = string to help make some of this more self-documenting.
But, again, not looking for this now. Just noting as it's been on my mind to review a general strategy for how and where we define our types - my sense is that dedicated files for each top-level package might be a sweet spot, with exception for model config and component prop types, which could continue to live with their model/comp source.
- Use `--` modifier when appending to validation-related classes. - Remove some remaining direct uses of the intents or colors in favor of the new per-level CSS vars. - Define additional base classes for validation messages. - Fixup changelog to note updates under correct version.
# Conflicts: # CHANGELOG.md
* Refactor form CSS classes to follow BEM conventions * Codev with Greg - Stop forcing form field info messages to a single line w/clipping - allow to wrap, apply styles used in former ViewManager override example. - Apply validation color to form field label on desktop, as was already done on mobile. - Improve non-minimal validation msg display to only use a tooltip if there are multiple messages. - Simplify some style rules via new BEM conventions --------- Co-authored-by: Anselm McClain <atm@xh.io>
|
We ended up pushing a bit farther on this, standardizing more of the classes to use the BEM conventions and cleaning up some additional jank.
Before release need to take another pass over the changelog and call out the CSS changes |
- New `--xh-intent-xxx-text-color` variants, customized to lighter color for dark theme to enhance legibility + contrast, used for form field validation colors - New set of `--xh-form-field-label` vars to support var-driven customization of FF labels w/o need for direct CSS selectors (on the just-changed classes) - Unwind weird/old setting of BP selectors based on (otherwise unused) form-field margin vars - Fix weird/old defaulting of `--xh-input` vars, which were looking to unqualified `--form-field` versions for their defaults.
|
|
||
| &.bp6-inline { | ||
| margin-right: var(--xh-form-field-margin-right); | ||
| margin-inline-end: 0; |
There was a problem hiding this comment.
It was surprising / weird that these vars existed but were only being applied here - pretty clearly wrong, looks like the original code was mine from 2018 so 🤷 what was going on.
I highly doubt that any apps were using these to customize, and if they were - well, the vars weren't doing what they claimed to do anyway.
| --xh-input-disabled-bg: var(--form-field-disabled-bg, rgba(206, 217, 224, 0.5)); // This and below currently matched from Blueprint defaults | ||
| --xh-input-disabled-text-color: var(--form-field-disabled-text-color, rgba(92, 112, 128, 0.5)); | ||
| --xh-input-disabled-bg: var(--input-disabled-bg, rgba(206, 217, 224, 0.5)); // This and below currently matched from Blueprint defaults | ||
| --xh-input-disabled-text-color: var(--input-disabled-text-color, rgba(92, 112, 128, 0.5)); |
There was a problem hiding this comment.
Very few apps use this general CSS variable convention of setting the vars without the xh prefix - we have had it from the beginning as it was (maybe still is?) a CSS vars best practice so you don't need to worry about specificity when declaring your value, but these defaults were just wrong and, again, highly doubt it will have an impact on apps.
- CSS vars for customization - Don't clip to single line on mobile (now consistent with desktop)
amcclain
changed the title
FormField styling
Accompanies HR PR: xh/hoist-react#4194 --------- Co-authored-by: Anselm McClain <atm@xh.io>
Hoist P/R Checklist
Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.
developbranch as of last change.breaking-changelabel + CHANGELOG if so.If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.